Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

T003: mols2grid example added #279

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

T003: mols2grid example added #279

wants to merge 1 commit into from

Conversation

e-mayo
Copy link

@e-mayo e-mayo commented Oct 18, 2022 •
edited by dominiquesydow
Loading

Description

A simple introductory example of mols2grid.display function was added in the T003_compound_unwanted_substructures.

Todos

Notable points that this PR has either accomplished or will accomplish.

  • Add an example of mols2grid

Reviewers:

  • Download and checkout this branch locally (installation) and run notebook to check if everything runs as expected
  • Check rendering on our website locally (instructions)
  • Check if CI runs for all OS (Linux/Windows/MacOS) and Python versions (check if GitHub Action fails are connected to T003, if not ignore). > CI passes for T003
  • Spellcheck and code review
  • Do we need to update the Table of Content, i.e. were any new sections added in the Theory or Practical part?

Questions

  • Question1

Status

  • Ready to go

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@dominiquesydow dominiquesydow changed the title mols2grid example added T003: mols2grid example added Oct 31, 2022
@dominiquesydow dominiquesydow added the enhancement New feature or request label Oct 31, 2022
@dominiquesydow
Copy link
Collaborator

Hi @e-mayo,

Thanks a lot for this addition to T003! 🚀
Introducing mols2grid here is super useful I think :)

I added a couple of TODOs for the reviewers to the PR description.
@yonghui-cc, would you like to take over some of the TODOs? Let me know if you have any questions!

We have a passing CI for T003 🥳

@yonghui-cc
Copy link
Contributor

yonghui-cc commented Nov 2, 2022 •
edited
Loading

Hi @e-mayo,

Thanks a lot for this addition to T003! 🚀 Introducing mols2grid here is super useful I think :)

I added a couple of TODOs for the reviewers to the PR description. @yonghui-cc, would you like to take over some of the TODOs? Let me know if you have any questions!

We have a passing CI for T003 🥳

Hi, @dominiquesydow,
I'm glad to review the nice notebook, but I have a problem here:
I try to run the 'import mols2grid' code but failed, so I realized there is something wrong with my environment. Then I create the env by installation, but it still failed in the Pip subprocess.

'''
image
'''

It's on macOS and I will try to find what's wrong here with the environment.

@dominiquesydow
Copy link
Collaborator

Hi @yonghui-cc,

Thanks for looking into this! Can you please check if you can install this via conda?
conda install -c conda-forge mols2grid

Or you reinstall the full environment based on this file:
https://github.com/e-mayo/teachopencadd/blob/add-mols2grid/devtools/test_env.yml

Let me know if this works for you, thank you!

@yonghui-cc
Copy link
Contributor

yonghui-cc commented Nov 17, 2022 •
edited
Loading

Hi, @dominiquesydow,

  • For the install problem: it worked when I install mols2grid independently. But it still failed for the full environment install:

mamba env create -f https://raw.githubusercontent.com/volkamerlab/TeachOpenCADD/master/devtools/test_env.yml
Error message:
image

Although the error happened, the teachopencadd env could still be created without mols2grid package, maybe also others we don't know.

  • For the code running: the notebook is great and worked well on my machine.

  • For the task rendering on the website locally: It failed to run make html
    Error message:

image

this may help for the first error: https://www.mail-archive.com/[email protected]/msg894683.html
For the second error, I try to fix the missing module, but there are more and more missing, I guess it's the sphinx version problem. Which version of sphinx on your machine?

@e-mayo
Copy link
Author

e-mayo commented Nov 20, 2022 via email

@yonghui-cc
Copy link
Contributor

yonghui-cc commented Dec 1, 2022 •
edited
Loading

Eduardo

Hi, @e-mayo, I tried the environment installment command line on Unbuntu OS, but the same problem happen.
image
So, we know that it's not an OS problem.

@e-mayo
Copy link
Author

e-mayo commented Dec 3, 2022

Hello!!

I have generated a new yml file. This works on my computer Windows [Version 10.0.22621.819]
Please test it under your systems and let me know if it works now.

test_env.txt

@dominiquesydow
Copy link
Collaborator

dominiquesydow commented Dec 5, 2022 •
edited
Loading

Hi @e-mayo and @yonghui-cc,
Thanks a lot!
The environment file works well for me (macOS) as well, the CI is picking up mols2grid without problems for all systems as well.

@yonghui-cc, can you please try installing the environment from a local yml file instead of from the GitHub URL (it cannot install the teachopencadd package because you do not have it downloaded I think) -- i.e. clone the repo first, cd into the teachopencadd folder and then do mamba env create -f devtools/test_env.yml -n toc-mols2grid.

@e-mayo I finally added the code review for the new pieces in the notebook, please let me know if you find this reasonable, thanks!

EDIT: Website rendering works nicely :)

dominiquesydow
dominiquesydow reviewed Dec 5, 2022
View reviewed changes
teachopencadd/talktorials/T003_compound_unwanted_substructures/talktorial.ipynb
@@ -122,23 +122,17 @@
"cell_type": "code",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove empty line before mols2grid as it is part of the third party import block (see the respective PEP).


Reply via ReviewNB

teachopencadd/talktorials/T003_compound_unwanted_substructures/talktorial.ipynb
@@ -122,23 +122,17 @@
"cell_type": "code",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to move the whole mols2grid topic to the end of the Substructure statistics section and look at the three most frequent substructures:

  • Michael-acceptor C=!@CC=[O,S]
  • Aliphatic-long-chain  [R0&D2][R0&D2][R0&D2][R0&D2]
  • Oxygen-nitrogen-single-bond [OR0,NR0][OR0,NR0]

Some minor text edits:

Dataframes > DataFrame

unwanted substructures, let's have a look


Reply via ReviewNB

teachopencadd/talktorials/T003_compound_unwanted_substructures/talktorial.ipynb
@@ -122,23 +122,17 @@
"cell_type": "code",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please run black-nb over this notebook (should streamline the formatting with the rest of the notebooks):

black-nb -l 99 teachopencadd/talktorials/T003_compound_unwanted_substructures/talktorial.ipynb

Reply via ReviewNB

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will be able to have a look at it only after the Friday. So expect some update by next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dominiquesydow dominiquesydow dominiquesydow left review comments

@yonghui-cc yonghui-cc Awaiting requested review from yonghui-cc

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@e-mayo @dominiquesydow @yonghui-cc